Improve notifications to fix commit notification and support release notification #34803
Conversation
GiteaBot
added
the
lgtm/need 2
github-actions
bot
added
modifies/api
GiteaBot
added
lgtm/need 1
models/migrations/v1_25/v321.go
Outdated
|
|
||
| updatedByIndex := schemas.NewIndex("idx_notification_updated_by", schemas.IndexType) | ||
| updatedByIndex.AddColumn("updated_by") | ||
| indices = append(indices, updatedByIndex) |
There was a problem hiding this comment.
Are these indices well-designed and correct?
You can't just add index to every column and expect that "oh it should work".
Does database work really work that way?
There was a problem hiding this comment.
This highlights a significant design flaw in the current table structure.
In this PR, we introduced a new column, release_id, to support release-type notifications.
GiteaBot
added
lgtm/done
services/convert/notification.go
Outdated
| @@ -71,7 +72,7 @@ func ToNotificationThread(ctx context.Context, n *activities_model.Notification) | |||
| url := n.Repository.HTMLURL() + "/commit/" + url.PathEscape(n.CommitID) | |||
| result.Subject = &api.NotificationSubject{ | |||
| Type: api.NotifySubjectCommit, | |||
| Title: n.CommitID, | |||
| Title: strings.TrimSpace(n.Commit.CommitMessage), | |||
There was a problem hiding this comment.
Is it fine to display the commit's full message to end users? Even if the message contains thousands of words?
| default: | ||
| if err := activities_model.CreateOrUpdateIssueNotifications(db.DefaultContext, opts.IssueID, opts.CommentID, opts.NotificationAuthorID, opts.ReceiverID); err != nil { | ||
| log.Error("Was unable to create issue notification: %v", err) | ||
| } |
There was a problem hiding this comment.
Should it panic in dev mode to warn developers that there is a mistake if the "opt" is not for "issue"?
| releaseIDIndex := schemas.NewIndex("idx_notification_release_id", schemas.IndexType) | ||
| releaseIDIndex.AddColumn("release_id") | ||
| indices = append(indices, releaseIDIndex) | ||
|
|
There was a problem hiding this comment.
The database design looks strange.
If I understand correctly, what you need is "a unique key to avoid duplication and help to mark the notification as read". I think it could be resolved by a more flexible approach like:
unique_key = 'commit-{CommitID}'
unique_key = 'release-{ReleaseID}'
unique_key = 'comment-{IssueID}-{CommentID}'
Then we only need one unique index (user_id, unique_key)
And, some legacy indices like status, source seem not able to help to improve querying performance because if user_id is used, then these indices won't be used.
And, the user_id index is redundancy because there is already (user_id, status, updated_unix). If you would take more care about performance, these problems should be handled.
This PR improved notifications.